-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Saturday, May 30, 2009 9:57 PM To: Chaithrika U S Cc: alsa-devel@alsa-project.org; davinci-linux-open- source@linux.davincidsp.com; Steve Chen; Naresh Medisetty; Pavel Kiryukhin Subject: Re: [alsa-devel] [PATCH v3 5/6] ARM: DaVinci: ASoC: Add mcasp support for DM646x
On Thu, May 28, 2009 at 05:11:08AM -0400, Chaithrika U S wrote:
The codec on the DM646x EVM is AIC32 which is connected to
Serializer0 and
Serializer1 of McASP0. The McASP0 consists of transmit and receive
sections
that may operate synchronized, or completely independently with
separate master
clocks, bit clocks, and frame syncs, and using different transmit
modes with
different bit-stream formats.
The particular configuration used for the EVM should not be relevant for the McASP driver.
Yes, agree.
+/* Left (even TDM Slot) Channel Status Register File*/ +#define DAVINCI_MCASP_DITCSRA_REG 0x100 +/* Right (odd TDM slot) Channel Status RegisterFile*/ +#define DAVINCI_MCASP_DITCSRB_REG 0x118
Interesting spacing in the comments...
- case DAVINCI_AUDIO_WORD_32:
fmt = 0x0F;break;- default:
return -1;- }
Should be a real error code.
Agree.
+static void davinci_hw_param(struct snd_pcm_substream *substream) +{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct davinci_audio_dev *dev = rtd->dai->cpu_dai->private_data;
It might be easier with a lot of these functions to just pass the davinci_audio_dev as a parameter.
OK.I will change this wherever possible.
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {/* bit stream is MSB first with no delay *//* DSP_B mode */Your driver consistently refers to itself as an I2S driver but it appears that in actual fact it only implements DSP mode B. This might be a bit confusing for users.
The hardware supports I2S, DSP A/B modes, currently it is configured in DSP B mode. May be changing all the function names from davinci_i2s_mcasp_XXX() to davinci_mcasp_xxx() will avoid confusion. Along with this, the file names can be changed to davinci-mcasp.c[h].
- /* Set the TX format : 24 bit right rotation, 32 bit slot, Pad 0
and LSB first */- mcasp_set_bits(dev->base + DAVINCI_MCASP_TXFMT_REG,
TXROT(6) | TXSSZ(15));I suspect looking at some of the indentation that you're writing this code with 4 character tabs.
+static int davinci_i2s_mcasp_probe(struct platform_device *pdev) +{
- struct evm_snd_platform_data *parray = pdev->dev.platform_data;
- struct davinci_pcm_dma_params *dma_data;
- struct resource *mem, *ioarea, *res;
- struct evm_snd_platform_data *pdata;
Why is the McASP driver using platform data called 'evm_snd_patform_data'? This suggests that there's some abstraction problem with the separation between the machine and McASP drivers.
Are the two DAIs directly tied to each other in hardware? If not it'd probably be better to have them registered as separate devices and probe separately so that if another chip comes along with a different set of DAIs it can be accommodated more readily - if the register interfaces stay consistent it may simply be a case of registering the new device.
Are you referring to the two DAI links in this driver? They are two instances of the same hardware, therefore tied to each other. For future chips, the instances may change. Therefore, the number of links should come from the platform data and hard coding it to 2 is not right. I will correct this.
- ret = snd_soc_register_dais(davinci_iis_mcasp_dai,
ARRAY_SIZE(davinci_iis_mcasp_dai));- if (ret != 0)
goto err_release_region;You should initialise dev within the DAI to be the struct device for the platform driver you were probed with. It might also be nice to tie this in to num_links somehow.
OK, will look into this.
Regards, Chaithrika