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).
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
