On Mon, May 21, 2012 at 05:33:42PM -0700, Qiao Zhou wrote:

Is there any suggestion/comment for this patch? Thanks!

Don't top post.

+config SND_PXA_SOC_SQU

•   tristate
  

•   select GENERIC_ALLOCATOR
  

The main reason I've left this is that I'm trying to decide what to say about the fact that it's not using dmaengine - why is it not doing so?

There's also a few smaller issues below:

•   for (i = 0; i < 2; i++) {
  

•           dint = SDISR(i);
  

•           if (dint) {
  

•                   substream = squ_data->stream[i];
  

•                   prtd = substream->runtime->private_data;
  

•                   if (SDISR(i) & 0x1)
  

•                           snd_pcm_period_elapsed(substream);
  

•                   else
  

•                           pr_debug("%s: SQU error on channel %d\n",
  

•                                    prtd->params->name, i);
  

Should be dev_err().

•                   SDISR(i) = 0;
  

•           }
  

•   }
  

•   return IRQ_HANDLED;
  

This interrupt handler unconditionally returns IRQ_HANDLED even if there were no interrupts reported. It should probably only do this if one of the tests in the loop returned true.

•   /* return if this is a bufferless transfer e.g.
  

•    * codec <--> BT codec or GSM modem -- lg FIXME */
  

•   if (!dma)
  

•           return 0;
  

Remove this, systems should use the framework features for this (and I don't think you're Liam...).

•   prtd = kzalloc(sizeof(struct pxa_runtime_data), GFP_KERNEL);
  

•   if (prtd == NULL) {
  

•           ret = -ENOMEM;
  

•           goto out;
  

•   }
  

devm_kzalloc().

+struct snd_soc_platform_driver pxa_squ_soc_platform = {

•   .ops = &pxa_squ_pcm_ops,
  

•   .pcm_new = pxa_squ_pcm_new,
  

•   .pcm_free = pxa_squ_pcm_free,
  

+}; +EXPORT_SYMBOL_GPL(pxa_squ_soc_platform);

This should never need to be exported.

•   squ_data = kzalloc(sizeof(struct pxa_squ_priv), GFP_KERNEL);
  

•   if (!squ_data)
  

•           return -ENOMEM;
  

devm_kzalloc().

•   squ_data->irq = pdata->irq;
  

Shouldn't the interrupt be supplied via a resource?

•   ret =
  

•       request_irq(squ_data->irq, pxa_squ_dma_irq, IRQF_DISABLED,
  

•                   "pxa-squ", pdev);
  

•   if (ret) {
  

•           pr_err("Wow!  Can't register IRQ for SQU\n");
  

dev_err() and print the error code.

+static int __init pxa_squ_pcm_modinit(void) +{

•   return platform_driver_register(&pxa_squ_driver);
  

+}

+module_init(pxa_squ_pcm_modinit);

module_platform_driver().