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

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

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

On Fri, Feb 03, 2012 at 10:09:24PM +0530, M R Swami Reddy wrote:

> ---
> Changes made in V2
> o Removed fll and vco reference frequency settings from _set_dai_pll()
> o Reworked chip enable and disable in _STANDBY and _OFF modes in
> m49453_set_bias_level, as per the review comments.

You say this is version 2 but there's been rather more than two versions
posted...

> +static int lm49453_set_dai_pll(struct snd_soc_dai *codec_dai, int pll_id,
> +                              int source, unsigned int freq_in,
> +                              unsigned int freq_out)
> +{

This is now a bit odd.

> +       state->in = freq_in;
> +       state->out = freq_out;

It stores but otherwise ignores the configuration which was passed in
(which seems more than a little odd).

> +       /* Always disable the PLL - it is not safe to leave it running
> +        * while reprogramming it.
> +        */
> +       snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG,
> +                           LM49453_PMC_SETUP_PLL_EN, 0);
> +
> +       if (!freq_in || !freq_out)
> +               return 0;
> +
> +
> +       /* All done, turn it on */
> +       snd_soc_update_bits(codec, LM49453_P0_PMC_SETUP_REG, pwr_mask,
> +                           pwr_mask);

Then if the PLL was already enabled it bounces the power briefly.
Really it seems like this should just be merged in with set_sysclk() -
you're not actually configuring the PLL at all, just turning it on and
off, at which point it's just another SYSCLK source.

Otherwise this seems good, just this clock configuration stuff to sort
out.

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

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