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

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

[Haojian Zhuang]

On Fri, May 18, 2012 at 10:39 AM, Qiao Zhou <zhouqiao@marvell.com> wrote:
> this patch adds ASoC pxa-squ DMA audio support.
>
> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> +/*
> + * Descriptor structure for PXA's SQU engine
> + * Note: this structure must always be aligned to a 16-byte boundary.
> + */
> +struct pxa_squ_desc {
> +       __iomem u32 byte_cnt;   /* byte count */
> +       __iomem u32 src_addr;   /* source address */
> +       __iomem u32 dst_addr;   /* target address */
> +       __iomem u32 nxt_desc;   /* next descriptor dress */
Why do you use __iomem at here?

> +};
> +

Re: [PATCH 1/2] ARM: MMP: add pxa-squ header file

[Haojian Zhuang]

On Fri, May 18, 2012 at 4:43 PM, Qiao Zhou <zhouqiao@marvell.com> wrote:
> add pxa-squ header file to support SQU DMA.
>
> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> ---
>  arch/arm/mach-mmp/include/mach/pxa-squ.h |   39 ++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
>  create mode 100644 arch/arm/mach-mmp/include/mach/pxa-squ.h
>
> diff --git a/arch/arm/mach-mmp/include/mach/pxa-squ.h b/arch/arm/mach-mmp/include/mach/pxa-squ.h
> new file mode 100644
> index 0000000..3ee187f
> --- /dev/null
> +++ b/arch/arm/mach-mmp/include/mach/pxa-squ.h
> @@ -0,0 +1,39 @@
> +#ifndef __MACH_SQU_H
> +#define __MACH_SQU_H
> +
> +#include <mach/addr-map.h>
> +
> +#define SQU_REGS_VIRT  (AXI_VIRT_BASE + 0xA0000)
> +#define SQU_REG(x)     (*((__iomem u32 *)(SQU_REGS_VIRT + (x))))
> +
> + /*SQU*/
> +#define SDSAR(x)               SQU_REG(0x810 + ((x) << 2))
> +#define SDDAR(x)               SQU_REG(0x820 + ((x) << 2))
> +#define SDNDPR(x)              SQU_REG(0x830 + ((x) << 2))
> +#define SDCR(x)                        SQU_REG(0x840 + ((x) << 2))
> +#define SDIMR(x)               SQU_REG(0x880 + ((x) << 2))
> +#define SDISR(x)               SQU_REG(0x8a0 + ((x) << 2))
> +#define SDCR_SSPMOD            (1 << 21)       /* SSPMod */
> +#define SDCR_FETCHND   (1 << 13)
> +#define SDCR_CHANEN            (1 << 12)       /* Channel Enable */
> +#define SDCR_DST_ADDR_INC      (0 << 4)
> +#define SDCR_DST_ADDR_HOLD     (0x2 << 4)
> +#define SDCR_SRC_ADDR_INC      (0 << 2)
> +#define SDCR_SRC_ADDR_HOLD     (0x2 << 2)
> +#define SDCR_DMA_BURST_4B      (0x0 << 6)
> +#define SDCR_DMA_BURST_8B      (0x1 << 6)
> +#define SDCR_DMA_BURST_16B     (0x3 << 6)
> +#define SDCR_DMA_BURST_1B      (0x5 << 6)
> +#define SDCR_DMA_BURST_2B      (0x6 << 6)
> +#define SDCR_DMA_BURST_32B     (0x7 << 6)
> +#define SDIMR_COMP                     (1 << 0)
> +
> +struct pxa_squ_platdata {
> +       char *pool_name;
> +       int irq;
> +       int period_max_out;             /* max period size for playback */
> +       int period_max_in;              /* max period size for capture */
> +       int buffer_max_out;             /* max buffer size for playback */
> +       int buffer_max_in;              /* max buffer size for capture */
> +};
> +#endif /* __MACH_SQU_H */
> --
> 1.7.4.1
>

Acked all.

Mark,

Would you mind to apply this patch series?

Thanks
Haojian

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

[Mark Brown]

[-- 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 --]