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).