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

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

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

On Fri, Feb 03, 2012 at 04:19:00AM -0800, Reddy, MR Swami wrote:

Fix your mailer to word wrap within paragraphs.  I'm sure I've mentioned
this before...

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

> The above code used to set the reference frequencies for FLL and VCO.
> Ideally, this code can be removed, because these registers are used
> for reference only.

You're saying that the PLL is actually not user configurable at all and
the above register writes have no effect?

> >> +       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;
> >> +       }

> The above code used to set for CLOCK type (ie either PLL or  FLL). The
> sysclk is PLL (HIGH FREQ mode) or FLL (LOW FREQ mode). So based on the
> sysclk, the BIT(4) will be set.

This doesn't address my comment (which you've deleted from my reply)
at all.  Why is this being done in hw_params()?  It appears to be
totally unrelated to hw_params().

The clocking configuration in this driver seems very confused, the
system clock for the device should be being configured by some
combinantion of set_sysclk() and set_pll() and hw_params() should do any
adjustment of the clock dividers and so on to configure the rest of the
chip for the sample rate based on the current clocking configuration.

> >> +       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;
> >> +       }

> Setting CHIP_EN bit as '0'. In the _STANDBY mode, CHIP_EN bit setting
> not needed and will remove in the next patch.

This also sounds broken, as I repeatedly said when reviewing previous
versions of the driver if disabling the chip does anything at all then
why don't you need to reenable the chip later?

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

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