[alsa-devel] [PATCH] ALSA: ASoc: Add regulator support to CS4270 codec driver

[Mark Brown]

On Wed, Nov 25, 2009 at 03:36:27PM +0100, Daniel Mack wrote:

> +	struct regulator *va_reg;

This should be three different supplies - there's separate digital and
control supplies, as well as a buffer supply.  Board designs frequently
split the analog and digital supplies.  The regulator API has bulk_
variants intended to make using multiple supplies en masse easier.

>  static int cs4270_dai_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_codec *codec = dai->codec;
>  	struct cs4270_private *cs4270 = codec->private_data;
> -	int reg6;
> +	int reg6, err;
>  
>  	reg6 = snd_soc_read(codec, CS4270_MUTE);
>  
>  	if (mute)
>  		reg6 |= CS4270_MUTE_DAC_A | CS4270_MUTE_DAC_B;
>  	else {
> +		if (cs4270->va_reg)
> +			regulator_enable(cs4270->va_reg);
> +

This looks wrong - why is the power being controlled in the mute
function?  If nothing else this is going to break recording since the
CODEC will only be unmuted during playback which means power will be cut
during record.

> +#ifdef CONFIG_REGULATOR
> +	/* get the regulator if there is one read<y for us */
> +	cs4270->va_reg = regulator_get(&pdev->dev, "va");
> +
> +	if (IS_ERR(cs4270->va_reg))
> +		cs4270->va_reg = NULL;
> +#endif

This isn't needed, the regulator API stubs itself out so you can just
unconditionally use it in consumer drivers.  The same applies to the
enable/disable calls, a simple driver like this should never need to
check to see if the API is enabled (this is why you don't need to ifdef
out the calls in the mute function).