On Thu, Feb 02, 2012 at 06:30:30AM -0800, Reddy, MR Swami wrote:
snd_soc_write(codec, LM49453_P0_FLL_REF_FREQL_REG, (state->in & 0xff));snd_soc_write(codec, LM49453_P0_FLL_REF_FREQH_REG,((state->in >> 8) & 0xff));snd_soc_write(codec, LM49453_P0_VCO_TARGETLL_REG,state->out & 0xff);snd_soc_write(codec, LM49453_P0_VCO_TARGETLH_REG,(state->out >> 8) & 0xff);snd_soc_write(codec, LM49453_P0_VCO_TARGETHL_REG,(state->out >> 16) & 0xff);snd_soc_write(codec, LM49453_P0_VCO_TARGETHH_REG,(state->out >> 24) & 0xff);
I see you've ignored my concern about not validating the inputs at all here.
switch (lm49453->sysclk) {case 12288000:case 26000000:case 19000000:/* PLL CLK slection */pll_clk = ~BIT(4);break;case 48000:case 32576:/* FLL CLK slection */pll_clk = BIT(4);break;default:return -EINVAL;}
Similarly for my concern about why you're updating the PLL configuration here - to repeat my previous comments shouldn't this be part of either the sysclk or PLL setup? It doesn't look anything to do with hw_params.
Please don't just ignore review comments, it's likely that if you send the same code again the same issues will be raised. At least reply to the mail if you think the review has missed something. It's not great finding the same issues over and over again...
case SND_SOC_BIAS_STANDBY:if (codec->dapm.bias_level == SND_SOC_BIAS_OFF)regcache_sync(lm49453->regmap);snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG,LM49453_PMC_SETUP_CHIP_EN, 0);break;case SND_SOC_BIAS_OFF:snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG,LM49453_PMC_SETUP_CHIP_EN, 0);break;}
This looks very wrong, you're setting the chip enable bit to the same value in both STANDBY and OFF.
+static int lm49453_resume(struct snd_soc_codec *codec) +{
struct lm49453_priv *lm49453 = snd_soc_codec_get_drvdata(codec);regcache_sync(lm49453->regmap);lm49453_set_bias_level(codec, SND_SOC_BIAS_STANDBY);lm49453_set_bias_level(codec, codec->dapm.suspend_bias_level);return 0;+}
If that's all you're doing to resume you shouldn't need suspend and resume functions at all, you're also syncing the cache in set_bias_level() and the subsystem will bring the device down to the lowest possible power level (off in the case of this device) before it suspends.