Re: [PATCH 1/2] ASoC: omap-mcbsp: Correct the DSP_B mode

[Peter Ujfalusi <peter.ujfalusi@nokia.com>]

On Tuesday 14 April 2009 12:31:33 ext Mark Brown wrote:
> On Tue, Apr 14, 2009 at 12:02:21PM +0300, Jarkko Nikula wrote:
> > The pairs must mach. It's clearly a bug if another end is have to set
> > different polarity than another. Those polarity defines are with
> > respect to the mode, not e.g. what's the McBSP default.
>
> Indeed.  However, the osk5912 board file that Peter pointed at as not
> needing any modifications for DSP_B already uses inverted frame clock
> for both CODEC and CPU.  In that case either both are wrong or it's just
> that the driver is limited in what it supports which is not a problem.

Well, I think the mcbsp module is quite - maybe too - flexible...
To have the DSP_B mode correctly (for the tvl320aic32 codec used in osk5912 
board) the FS polarity has to be handled by the mcbsp as it has been inverted. 
If we don't do this, there is no way to have the MSB at the correct place (it 
has to be available when the FS is high).

The DSP_A mode can use the FS polarity 'correctly' - as it is. Or we can also 
consider to require to invert the FS polarity, than add 1 bit delay for DSP_A 
mode.

So:
a) The proposal in the series
DSP_B mode (the MSB is transmitted when the FS is high, the length for the 
pulse is 1):
	case SND_SOC_DAIFMT_DSP_B:
		regs->srgr2	|= FPER(wlen * channels - 1);
		regs->srgr1	|= FWID(0);
		break;

	case SND_SOC_DAIFMT_DSP_B:
		/* 0-bit data delay */
		regs->rcr2      |= RDATDLY(0);
		regs->xcr2      |= XDATDLY(0);
		break;

	case SND_SOC_DAIFMT_NB_IF:
		regs->pcr0	|= CLKXP | CLKRP;
		break;

DSP_A mode (the MSB is transmitted when the FS went low, the length for the 
pulse is still 1, but the FS stays low for (wlen * channels - 1) cycles):
	case SND_SOC_DAIFMT_DSP_A:
		regs->srgr2	|= FPER(wlen * channels - 1);
		regs->srgr1	|= FWID(wlen * channels - 2);
		break;

	case SND_SOC_DAIFMT_DSP_A:
		/* 0-bit data delay */
		regs->rcr2      |= RDATDLY(0);
		regs->xcr2      |= XDATDLY(0);
		break;

	case SND_SOC_DAIFMT_IB_IF:
		break;

b) twist in DSP_A mode.
The DSP_B mode is the same as it is in a)

DSP_A mode (the MSB is transmitted when the FS went low, the length for the 
pulse is still 1, but the FS stays low for (wlen * channels - 1) cycles):
	case SND_SOC_DAIFMT_DSP_A:
		regs->srgr2	|= FPER(wlen * channels - 1);
		regs->srgr1	|= FWID(0);
		break;

	case SND_SOC_DAIFMT_DSP_A:
		/* 0-bit data delay */
		regs->rcr2      |= RDATDLY(1);
		regs->xcr2      |= XDATDLY(1);
		break;

	case SND_SOC_DAIFMT_NB_IF:
		regs->pcr0	|= CLKXP | CLKRP;
		break;

Both a) and b) would be OK for the DSP_A mode...

c)
Remove the SND_SOC_DAIFMT_* (DSP_A, DSP_B, I2S and NB_NF, NB_IF, IB_NF, IB_IF) 
configuration from the omap_mcbsp_dai_set_dai_fmt, move it to the 
omap_mcbsp_dai_hw_params function and have something like these there:

switch (mcbsp_data->fmt &
			(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK)) {
case SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF:
	...
	break;
case SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_NB_NF:
	...
	break;
case SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_NB_IF:
	...
	break;
case SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF:
	...
	break;
case SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_IF:
	...
	break;
case SND_SOC_DAIFMT_DSP_A | SND_SOC_DAIFMT_NB_NF:
	...
	break;
...

}

and so on. I'm not sure if all of the combinations are valid, but it can be 
done like this also. It is another thing how actually the mcbsp would be 
configured for these modes - for DSP_B the FS polarity has to be inverted to 
get the MSB in a correct place, but it would be correct from the 
snd_soc_dai_set_fmt point of view, or something........


-- 
Péter