[alsa-devel] [PATCH ] ASoC: Add support for TI LM49453 Audio codec
Mark Brown
broonie at opensource.wolfsonmicro.com
Fri Feb 3 14:25:04 CET 2012
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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
Url : http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20120203/61cd1f1d/attachment.sig
More information about the Alsa-devel
mailing list