On Thu, Apr 02, 2009 at 09:47:18PM -0400, Naresh Medisetty wrote:
This all looks pretty good - the comments below are all pretty small, mostly coding style and questions.
- /* programming GBLCTL needs to read back from GBLCTL and verfiy */
- /* loop count is to avoid the lock-up */
- for (i = 0; (i < 1000) && ((mcasp_get_reg(regs) & val) != val); i++);
Should this warn if the loop times out? For legibility I'd rewrite as:
for (i = 0; i < 1000; i++) if ((mcasp_get_reg(regs) & val) == val) break;
The ; should be on a separate line at least.
+int davinci_i2s_startup(struct snd_pcm_substream *substream,
struct snd_soc_dai *dai)+{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
- struct snd_soc_dai *cpu_dai = rtd->dai->cpu_dai;
- struct davinci_audio_dev *dev = rtd->dai->cpu_dai->private_data;
- cpu_dai->dma_data = dev->dma_params[substream->stream];
cpu_dai is passed in as the dai parameter, no need to look it up like this (in the past it wasn't - most of the code hasn't yet been fixed up to take advantage of this yet).
+int davinci_i2s_mcasp_probe(struct platform_device *pdev,
struct snd_soc_dai *dai)
Indentation?
mem = platform_get_resource(pdev, IORESOURCE_MEM, link_cnt);if (!mem) {dev_err(&pdev->dev, "no mem resource?\n");ret = -ENODEV;}ioarea = request_mem_region(mem->start,(mem->end - mem->start) + 1, pdev->name);if (!ioarea) {dev_err(&pdev->dev, "Audio region already claimed\n");ret = -EBUSY;}
Should these be jumping to the unwind code?
dev = kzalloc(sizeof(struct davinci_audio_dev) *card->num_links, GFP_KERNEL);if (!dev) {return -ENOMEM;goto err_release_region;}
return followed by goto?
dma_data = kzalloc(sizeof(struct davinci_pcm_dma_params) *(card->num_links << 1), GFP_KERNEL);if (!dma_data)goto err_release_dev;
Do you need to set ret here?
- }
+} +static void davinci_hw_iis_param(struct snd_pcm_substream *substream)
Blank line between functions, please.
- if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
/* bit stream is MSB first */mcasp_set_bits(dev->base + DAVINCI_MCASP_AHCLKXCTL_REG,AHCLKXE);
For I2S you should have one BCLK before MSB - is that happening (it may well be, I've no knowledge of your hardware)?
- } else {
/* bit stream is MSB first with no delay */mcasp_set_bits(dev->base + DAVINCI_MCASP_RXFMT_REG,FSRDLY(1) | RXORD);
The comment and code don't look like they match up (but again, I don't have any knowledge of this specific hardware).