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@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx>
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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
