[alsa-devel] [PATCH v8 2/3] ASoC: add es8328 codec driver

[Mark Brown]

On Tue, Jul 15, 2014 at 09:41:33AM +0800, Sean Cross wrote:

> Add a codec driver for the Everest ES8328.  It supports two separate audio
> outputs and two separate audio inputs.

This looks mostly fine so I've applied it, there are some issues below -
please send followups fixing them.

> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> +		reg = ES8328_DACCONTROL2;
> +
> +	else
> +		reg = ES8328_ADCCONTROL5;

Coding style, extra blank line in the middle of the if ().

> +	switch (params_rate(params)) {
> +	case 8000:
> +		val = es8328_sample_ratios[ES8328_RATE_8019];
> +		break;
> +	case 11025:
> +		val = es8328_sample_ratios[ES8328_RATE_11025];
> +		break;
> +	case 22050:
> +		val = es8328_sample_ratios[ES8328_RATE_22050];
> +		break;
> +	case 44100:
> +		val = es8328_sample_ratios[ES8328_RATE_44100];
> +		break;
> +	default:
> +		dev_err(codec->dev, "%s: unknown rate %d\n",
> +			 __func__, params_rate(params));
> +		return -EINVAL;
> +	}

The array serves no function given that you're never iterating over it
or anything - either have the array be a lookup table or just put the
constants in the switch statement directly.  You're also calling the
array "_sample_ratios" but it actually appears to be sample rates.

> +	snd_soc_write(codec, ES8328_MASTERMODE, ES8328_MASTERMODE_MSC);
> +
> +	clk_rate = clk_get_rate(es8328->clk);
> +	if (clk_rate == ES8328_SYSCLK_RATE_1X)
> +		snd_soc_write(codec, ES8328_MASTERMODE,
> +				ES8328_MASTERMODE_MSC);
> +	else
> +		snd_soc_write(codec, ES8328_MASTERMODE,
> +				ES8328_MASTERMODE_MCLKDIV2 |
> +				ES8328_MASTERMODE_MSC);

update_bits().

> +		snd_soc_write(codec, ES8328_CHIPPOWER,
> +				ES8328_CHIPPOWER_ADCDIG_OFF |
> +				ES8328_CHIPPOWER_DACDIG_OFF |
> +				ES8328_CHIPPOWER_ADCSTM_RESET |
> +				ES8328_CHIPPOWER_DACSTM_RESET |
> +				ES8328_CHIPPOWER_ADCPLL_OFF |
> +				ES8328_CHIPPOWER_DACPLL_OFF |
> +				ES8328_CHIPPOWER_ADCVREF_OFF |
> +				ES8328_CHIPPOWER_DACVREF_OFF);
> +		break;

This looks like most if not all of it should be in DAPM, especially the
DAC and ADC power - the device is wasting power on the ADC during
playback for example.  Looking at the code I suspect that some of these
should be done as supply widgets.

> +static int es8328_suspend(struct snd_soc_codec *codec)
> +{
> +	es8328_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +	return 0;
> +}
> +
> +static int es8328_resume(struct snd_soc_codec *codec)
> +{
> +	struct regmap *regmap = dev_get_regmap(codec->dev, NULL);
> +
> +	regcache_mark_dirty(regmap);
> +	regcache_sync(regmap);
> +
> +	es8328_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +	return 0;
> +}

But leave the regulators and clocks enabled?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140717/592bb4d9/attachment.sig>