[alsa-devel] [PATCH 2/3] ASoC: Add WM8990 driver

[Takashi Iwai]

At Thu,  5 Jun 2008 09:54:06 +0100,
Mark Brown wrote:
> 
> The WM8990 is a highly integrated ultra-low power hi-fi codec designed
> for handsets rich in multimedia features such as mobile TV, digital
> audio playback and gaming.
> 
> The bulk of this driver was written by Liam Girdwood with some
> additional development and updates for new ASoC APIs by me.
> 
> Signed-off-by: Liam Girdwood <lg at opensource.wolfsonmicro.com>
> Signed-off-by: Mark Brown <broonie at opensource.wolfsonmicro.com>

Hm, I obviously didn't review it in the last cycle.

> +#include <linux/kernel.h>

Heh, the same issue as wm8510.

> +/*
> + * wm8990 register cache.  Note that register 0 is not included in the
> + * cache.
> + */
> +static const u16 wm8990_reg[] = {
> +    /*0x8990,*/     /* R0  - Reset */

Ditto.

> +/*
> + * read wm8990 register cache
> + */
> +static inline unsigned int wm8990_read_reg_cache(struct snd_soc_codec *codec,
> +	unsigned int reg)
> +{
> +	u16 *cache = codec->reg_cache;
> +	BUG_ON(reg < 1 || reg > (ARRAY_SIZE(wm8990_reg) + 1));

Isn't it 
	BUG_ON(reg < 1 || reg > ARRAY_SIZE(wm8990_reg));
??

But at the first place this is bad, because codec_reg_show() in
soc-core.c always loops from 0 to codec->reg_cache_size-1.
At least, reg == 0 should be simply ignored.


> +	return cache[reg - 1];
> +}
> +
> +/*
> + * write wm8990 register cache
> + */
> +static inline void wm8990_write_reg_cache(struct snd_soc_codec *codec,
> +	unsigned int reg, unsigned int value)
> +{
> +	u16 *cache = codec->reg_cache;
> +	BUG_ON(reg < 1 || reg > (ARRAY_SIZE(wm8990_reg) + 1));

Ditto.

> +static int wm8990_set_bias_level(struct snd_soc_codec *codec,
> +	enum snd_soc_bias_level level)
(snip)
> +	case SND_SOC_BIAS_STANDBY:
> +		if (codec->bias_level == SND_SOC_BIAS_OFF) {
> +			/* Enable all output discharge bits */
> +			wm8990_write(codec, WM8990_ANTIPOP1, WM8990_DIS_LLINE |
> +				WM8990_DIS_RLINE | WM8990_DIS_OUT3 |
> +				WM8990_DIS_OUT4 | WM8990_DIS_LOUT |
> +				WM8990_DIS_ROUT);
> +
> +			/* Enable POBCTRL, SOFT_ST, VMIDTOG and BUFDCOPEN */
> +			wm8990_write(codec, WM8990_ANTIPOP2, WM8990_SOFTST |
> +				     WM8990_BUFDCOPEN | WM8990_POBCTRL |
> +				     WM8990_VMIDTOG);
> +
> +			/* Delay to allow output caps to discharge */
> +			schedule_timeout_interruptible(msecs_to_jiffies(300));

Why interruptible?  Maybe msleep() is better for such a purpose?

> +
> +			/* Disable VMIDTOG */
> +			wm8990_write(codec, WM8990_ANTIPOP2, WM8990_SOFTST |
> +				     WM8990_BUFDCOPEN | WM8990_POBCTRL);
> +
> +			/* disable all output discharge bits */
> +			wm8990_write(codec, WM8990_ANTIPOP1, 0);
> +
> +			/* Enable outputs */
> +			wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x1b00);
> +
> +			schedule_timeout_interruptible(msecs_to_jiffies(50));

Ditto.

> +
> +			/* Enable VMID at 2x50k */
> +			wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x1f02);
> +
> +			schedule_timeout_interruptible(msecs_to_jiffies(100));
> +
> +			/* Enable VREF */
> +			wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x1f03);
> +
> +			schedule_timeout_interruptible(msecs_to_jiffies(600));
> +
> +			/* Enable BUFIOEN */
> +			wm8990_write(codec, WM8990_ANTIPOP2, WM8990_SOFTST |
> +				     WM8990_BUFDCOPEN | WM8990_POBCTRL |
> +				     WM8990_BUFIOEN);
> +
> +			/* Disable outputs */
> +			wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x3);
> +
> +			/* disable POBCTRL, SOFT_ST and BUFDCOPEN */
> +			wm8990_write(codec, WM8990_ANTIPOP2, WM8990_BUFIOEN);
> +		} else {
> +			/* ON -> standby */
> +
> +		}
> +		break;
> +
> +	case SND_SOC_BIAS_OFF:
> +		/* Enable POBCTRL and SOFT_ST */
> +		wm8990_write(codec, WM8990_ANTIPOP2, WM8990_SOFTST |
> +			WM8990_POBCTRL | WM8990_BUFIOEN);
> +
> +		/* Enable POBCTRL, SOFT_ST and BUFDCOPEN */
> +		wm8990_write(codec, WM8990_ANTIPOP2, WM8990_SOFTST |
> +			WM8990_BUFDCOPEN | WM8990_POBCTRL |
> +			WM8990_BUFIOEN);
> +
> +		/* mute DAC */
> +		val = wm8990_read_reg_cache(codec, WM8990_DAC_CTRL);
> +		wm8990_write(codec, WM8990_DAC_CTRL, val | WM8990_DAC_MUTE);
> +
> +		/* Enable any disabled outputs */
> +		wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x1f03);
> +
> +		/* Disable VMID */
> +		wm8990_write(codec, WM8990_POWER_MANAGEMENT_1, 0x1f01);
> +
> +		schedule_timeout_interruptible(msecs_to_jiffies(300));

Ditto.

> +static int wm8990_init(struct snd_soc_device *socdev)
> +{
> +	struct snd_soc_codec *codec = socdev->codec;
> +	u16 reg;
> +	int ret = 0;
> +
> +	codec->name = "WM8990";
> +	codec->owner = THIS_MODULE;
> +	codec->read = wm8990_read_reg_cache;
> +	codec->write = wm8990_write;
> +	codec->set_bias_level = wm8990_set_bias_level;
> +	codec->dai = &wm8990_dai;
> +	codec->num_dai = 2;
> +	codec->reg_cache_size = sizeof(wm8990_reg);

A wrong value assignment in this file, too.  But, ARRAY_SIZE() is also
wrong since the max reg value is ARRAY_SIZE().  ARRAY_SIZE()+1 is the
right value, but then it's really unclear what it means...


thanks,

Takashi