Re: [alsa-devel] [PATCH 3/5] SOUND: SOC: CODECS: Add support for the TWL4030 audio codec

[Mark Brown <broonie@sirena.org.uk>]

On Thu, Sep 04, 2008 at 01:26:32PM +0300, Jarkko Nikula wrote:

> > +static void twl4030_dump_registers(void)
> > +{
> This is not needed since there is already nice function
> for it: sound/soc/soc-core.c: codec_reg_show.

It's doing something slighly different and dumping the chip registers
rather than the register cache (the read() operation for this chip is
_read_reg_cache()) but yes, should be sensible to drop it.

> > +static void twl4030_power_down(struct snd_soc_codec *codec)
> > +{
> ...
> > +	udelay(10);
> > +}
> REVISIT comment for these kind of magic delays if doesn't work without.

Alternatively, if writes to CODECPDZ are always supposed to have delays
after them according to the datasheet perhaps a function to wrap up the
write plus delay would be a good idea?

> > +static int twl4030_set_dai_sysclk(struct snd_soc_dai *codec_dai,
> > +		int clk_id, unsigned int freq, int dir)
> > +{
> ...
> > +
> > +	infreq |= APLL_EN;
> > +	twl4030_write(codec, REG_APLL_CTL, infreq);
> > +
> > +	return 0;
> > +}

> If this actually place for set_pll callback if one wants to manage PLL
> (APLL_EN bit) dynamically?

Might be nice, yes.  That said, how much flexibility is there regarding
the PLL on this part?  If the configuration is determined very strongly
by the audio configuration and it's likely that the configuration is
entirely determined by the current audio status then it may not be worth
exposing the decision making to the user, at least for a first pass.
The impression I got from the code was that it was very fixed purpose.

If you do do this then make sure it interacts well with power_up() and
power_down().