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

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

[Mark Brown]

[-- 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 --]

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

[M R Swami Reddy]

Mark Brown 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...

Thats right. I started numbering the patch version with previous one, so 
mentioned it as v2. The version number will be continued.


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

I will update it as per the other drivers formate.


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

Will be removed in the next patch.


 > > +       /* 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.


Ok. The PLL disable code will be removed and the PLL enable code will be moved 
to _sysclk().

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

OK. Thank you. Do I need to resend the patch.

Thanks
Swami