Re: [PATCH ] ASoC: Add support for TI LM49453 Audio codec

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3086 bytes --]

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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]