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