Re: [PATCH v5 2/3] ASoC: add es8328 codec driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

On Fri, Jun 20, 2014 at 02:14:18PM +0800, Sean Cross wrote:

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

Idiomatically this is just a single if/else, if you weren't doing that
then it ought to be a switch but really there's no need to do something
non-idiomatic.

> +	/* Master serial port mode */
> +	snd_soc_write(codec, ES8328_MASTERMODE,
> +			ES8328_MASTERMODE_MCLKDIV2 |
> +			ES8328_MASTERMODE_MSC);

It seems a bit unfortunate that set_sysclk() doesn't manage MCLKDIV2 -
if that isn't badly named the driver could easily support both the
current fixed SYSCLK rate and twice that.

> +	case SND_SOC_BIAS_PREPARE:
> +		/* VREF, VMID=2x50k, digital enabled */
> +		snd_soc_write(codec, ES8328_CHIPPOWER, pwr_reg);
> +		snd_soc_write(codec, ES8328_CONTROL1,
> +				cc1_reg |
> +				ES8328_CONTROL1_VMIDSEL_50k |
> +				ES8328_CONTROL1_ENREF);

snd_soc_update_bits() as I'm fairly sure I said last time.

> +static int es8328_suspend(struct snd_soc_codec *codec)
> +{
> +       es8328_set_bias_level(codec, SND_SOC_BIAS_OFF);
> +       return 0;
> +}

Neither this nor set_bias_level() restores the register cache after
suspend - this will mean suspend and resume is broken in systems where
power is removed from the device over suspend.

> +	ret = devm_regulator_bulk_get(codec->dev, ARRAY_SIZE(es8328->supplies),
> +				es8328->supplies);
> +	if (ret) {
> +		dev_err(codec->dev, "!!! Unable to get regulators\n");
> +		return ret;
> +	}

Print the error code and format the messages normally - none of this
!!! stuff please.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]