On Wed, May 13, 2009 at 09:59:20PM -0400, Jon Smirl wrote:
This looks mostly good - a lot of the issues below are fairly nitpicky,
though the issue with the of_simple integration needs to be dealt with.
> select SND_SOC_SSM2602 if I2C
> + select SND_SOC_STAC9766
> select SND_SOC_TLV320AIC23 if I2C
This needs to be '... if SND_SOC_AC97_BUS' as for all the other AC97
CODECs.
> +{
> + u16 *cache = codec->reg_cache;
> +
> + if (reg > AC97_STAC_PAGE0) {
> + stac9766_ac97_write(codec, AC97_INT_PAGING, 0);
> + soc_ac97_ops.write(codec->ac97, reg, val);
> + stac9766_ac97_write(codec, AC97_INT_PAGING, 1);
> + return 0;
This could use a comment explaining what's going on with the paging -
I'd expect page 0 to be the default page.
> +static int ac97_digital_prepare(struct snd_pcm_substream *substream,
> + struct snd_soc_dai *dai)
> +{
> + struct snd_soc_codec *codec = dai->codec;
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + unsigned short reg, vra;
> +
> + stac9766_ac97_write(codec, AC97_SPDIF, 0x2002);
> +
> + vra = stac9766_ac97_read(codec, AC97_EXTENDED_STATUS);
> +
> + vra |= 0x5; /* Enable VRA and SPDIF out */
> + printk("var is %x\n", vra);
This looks like debug code that can be removed or turned into a
pr_debug()?
> +static int ac97_digital_trigger(struct snd_pcm_substream *substream,
> + int cmd, struct snd_soc_dai *dai)
Another indentation problem?
> + switch (cmd) {
> + case SNDRV_PCM_TRIGGER_STOP:
> + vra = stac9766_ac97_read(codec, AC97_EXTENDED_STATUS);
> + vra &= !0x04;
> + //stac9766_ac97_write(codec, AC97_EXTENDED_STATUS, vra);
> + break;
Debug code that could be cleaned up?
> +static int stac9766_set_bias_level(struct snd_soc_codec *codec,
> + enum snd_soc_bias_level level)
> +{
> + switch (level) {
> + case SND_SOC_BIAS_ON: /* full On */
> + stac9766_ac97_write(codec, AC97_POWERDOWN, 0x0000);
> + break;
> + case SND_SOC_BIAS_PREPARE: /* partial On */
> + stac9766_ac97_write(codec, AC97_POWERDOWN, 0x0000);
> + break;
You could skip the writes here - they're just doing the same thing as
standby.
> +static int stac9766_codec_resume(struct platform_device *pdev)
> +{
> + struct snd_soc_device *socdev = platform_get_drvdata(pdev);
> + struct snd_soc_codec *codec = socdev->card->codec;
> + u16 id;
> +
> + /* give the codec an AC97 warm reset to start the link */
> + codec->ac97->bus->ops->warm_reset(codec->ac97);
> + id = soc_ac97_ops.read(codec->ac97, AC97_VENDOR_ID2);
> + if (id != 0x4c13) {
> + printk(KERN_ERR "stac9766 failed to resume");
> + return -EIO;
> + }
Shouldn't this use the reset function?
> + .rates = SNDRV_PCM_RATE_8000_48000,
> + .formats = SNDRV_PCM_FMTBIT_S8 | SNDRV_PCM_FMTBIT_S16_BE |
> + SNDRV_PCM_FMTBIT_S24_BE | SNDRV_PCM_FMTBIT_S32_BE,
Should use the standard define for AC97 formats here, and for the other
DAIs.
> + snd_soc_add_controls(codec, stac9766_snd_ac97_controls, ARRAY_SIZE(
> + stac9766_snd_ac97_controls));
Odd place to wrap...
> +static int __init stac9766_probe(struct platform_device *pdev)
> +{
> + snd_soc_register_dais(stac9766_dai, ARRAY_SIZE(stac9766_dai));
The same issues as apply to the WM9712 change apply here.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
