[alsa-devel] [PATCH V1 10/13] Codec for STAC9766 used on the Efika

[Mark Brown]

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.