On 4/9/2009 11:06 PM, Mark Brown wrote:
On Thu, Apr 09, 2009 at 10:54:37PM +0900, Joonyoung Shim wrote:
The PCM voice interface has two modes
- PCM mode1 : This uses the rising edge of the clock signal
- PCM mode2 : This uses the falling edge of the clock signal
PCM mode1 and mode2 have a look of DSP_A and DSP_B, so we used DSP_A and DSP_B.
That sounds wrong - the difference between modes A and B is an extra cycle on BCLK after LRP, not the polarity of the signal. I'd expect that it should be one or the other of these modes with the option to invert BCLK.
Oh, i see. I will use "DAI hardware signal inversions" defines.
+static int twl4030_voice_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_device *socdev = rtd->socdev;
- struct snd_soc_codec *codec = socdev->card->codec;
- u8 infreq;
- /* If the system master clock is not 26MHz, the voice PCM interface is
* not avilable.*/- infreq = twl4030_read_reg_cache(codec, TWL4030_REG_APLL_CTL)
& TWL4030_APLL_INFREQ;- if (infreq != TWL4030_APLL_INFREQ_26000KHZ)
return -EPERM;- return 0;
+}
It's probably worth a comment here telling users that they'll need to call set_sysclk() in their init() function rather than hw_params() - otherwise this might get called before the clock is set up.
It seems better that i remove startup function and add set_sysclk function, and inform supporting only 26MHz system master clock through comment. i think it is not important where set_sysclk() is called, the voice PCM interface needs just 26MHz system master clock.
/* change rate and set CODECPDZ */twl4030_codec_enable(codec, 0);twl4030_write(codec, TWL4030_REG_CODEC_MODE, mode);twl4030_codec_enable(codec, 1);Hrm. This codec_enable() call looks fishy - what's the effect if the other DAI is running when the voice DAI is configured? Though there may be no way round this...
Frankly, i don't know whether this codec_enable() is called. The existing the HIFI DAI calls this codec_enable(). If this call is unnecessary i think it seems better to remove it.
+struct snd_soc_dai twl4030_dai[] = { +{ .name = "twl4030",
The { should ideally be on a line by itself (for both DAIs).
Ok, i will fix it.
Thanks for you review.