Re: [PATCH 2/2] ASoC: add pxa-squ dma support

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2641 bytes --]

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().

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]