Re: [PATCH 1/1] ASoC: TWL4030: Add support Voice DAI

[Joonyoung Shim <jy0922.shim@samsung.com>]

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.