Re: [PATCH 2/4] ASoC: mmp: add audio dma support

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Vinod Koul]

On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
> mmp-pcm handle audio dma based on dmaengine
> Support mmp and pxa910
Looks like this is *not* using soc-dmaengine library, why?
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@marvell.com>
> Signed-off-by: Leo Yan <leoy@marvell.com>
> Signed-off-by: Qiao Zhou <zhouqiao@marvell.com>
> ---
>  include/linux/platform_data/mmp_audio.h |   22 ++
>  sound/soc/pxa/Kconfig                   |    8 +
>  sound/soc/pxa/Makefile                  |    2 +
>  sound/soc/pxa/mmp-pcm.c                 |  448 +++++++++++++++++++++++++++++++
>  4 files changed, 480 insertions(+), 0 deletions(-)
>  create mode 100644 include/linux/platform_data/mmp_audio.h
>  create mode 100644 sound/soc/pxa/mmp-pcm.c
> 
> diff --git a/include/linux/platform_data/mmp_audio.h b/include/linux/platform_data/mmp_audio.h
> new file mode 100644
> index 0000000..0f25d16
> --- /dev/null
> +++ b/include/linux/platform_data/mmp_audio.h
> @@ -0,0 +1,22 @@
> +/*
> + *  MMP Platform AUDIO Management
> + *
> + *  Copyright (c) 2011 Marvell Semiconductors Inc.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + */
> +
> +#ifndef MMP_AUDIO_H
> +#define MMP_AUDIO_H
> +
> +struct mmp_audio_platdata {
> +	u32 period_max_capture;
> +	u32 buffer_max_capture;
> +	u32 period_max_playback;
> +	u32 buffer_max_playback;
> +};
> +
> +#endif /* MMP_AUDIO_H */
> diff --git a/sound/soc/pxa/Kconfig b/sound/soc/pxa/Kconfig
> index a0f7d3c..a516068 100644
> --- a/sound/soc/pxa/Kconfig
> +++ b/sound/soc/pxa/Kconfig
> @@ -8,6 +8,14 @@ config SND_PXA2XX_SOC
>  	  the PXA2xx AC97, I2S or SSP interface. You will also need
>  	  to select the audio interfaces to support below.
>  
> +config SND_MMP_SOC
> +	bool "Soc Audio for Marvell MMP chips"
> +	depends on ARCH_MMP
> +	select SND_ARM
> +	help
> +	  Say Y if you want to add support for codecs attached to
> +	  the MMP SSPA interface.
> +
>  config SND_PXA2XX_AC97
>  	tristate
>  	select SND_AC97_CODEC
> diff --git a/sound/soc/pxa/Makefile b/sound/soc/pxa/Makefile
> index af35762..f913e9b 100644
> --- a/sound/soc/pxa/Makefile
> +++ b/sound/soc/pxa/Makefile
> @@ -3,11 +3,13 @@ snd-soc-pxa2xx-objs := pxa2xx-pcm.o
>  snd-soc-pxa2xx-ac97-objs := pxa2xx-ac97.o
>  snd-soc-pxa2xx-i2s-objs := pxa2xx-i2s.o
>  snd-soc-pxa-ssp-objs := pxa-ssp.o
> +snd-soc-mmp-objs := mmp-pcm.o
>  
>  obj-$(CONFIG_SND_PXA2XX_SOC) += snd-soc-pxa2xx.o
>  obj-$(CONFIG_SND_PXA2XX_SOC_AC97) += snd-soc-pxa2xx-ac97.o
>  obj-$(CONFIG_SND_PXA2XX_SOC_I2S) += snd-soc-pxa2xx-i2s.o
>  obj-$(CONFIG_SND_PXA_SOC_SSP) += snd-soc-pxa-ssp.o
> +obj-$(CONFIG_SND_MMP_SOC) += snd-soc-mmp.o
>  
>  # PXA Machine Support
>  snd-soc-corgi-objs := corgi.o
> diff --git a/sound/soc/pxa/mmp-pcm.c b/sound/soc/pxa/mmp-pcm.c
> new file mode 100644
> index 0000000..abafbb9
> --- /dev/null
> +++ b/sound/soc/pxa/mmp-pcm.c
> @@ -0,0 +1,448 @@
> +/*
> + * linux/sound/soc/pxa/mmp-pcm.c
> + *
> + * Copyright (C) 2011 Marvell International Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
> +#include <linux/platform_data/mmp_dma.h>
> +#include <linux/platform_data/mmp_audio.h>
> +#include <sound/pxa2xx-lib.h>
> +#include <sound/core.h>
> +#include <sound/pcm.h>
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +#include <mach/sram.h>
> +
> +struct mmp_runtime_data {
> +	int ssp_id;
> +	u32 period_size;
> +	u32 pointer;
> +	struct resource *dma_res;
> +	struct mmp_tdma_data tdma_data;
> +	struct gen_pool *gpool;
> +	struct snd_pcm_substream *substream;
> +	struct dma_chan *dma_chan;
> +	struct dma_async_tx_descriptor *desc;
> +};
> +
> +#define MMP_PCM_INFO (SNDRV_PCM_INFO_MMAP |	\
> +		SNDRV_PCM_INFO_MMAP_VALID |	\
> +		SNDRV_PCM_INFO_INTERLEAVED |	\
> +		SNDRV_PCM_INFO_PAUSE |		\
> +		SNDRV_PCM_INFO_RESUME)
> +
> +#define MMP_PCM_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \
> +			 SNDRV_PCM_FMTBIT_S24_LE | \
> +			 SNDRV_PCM_FMTBIT_S32_LE)
> +
> +static struct snd_pcm_hardware mmp_pcm_hardware[] = {
> +	{
> +		.info			= MMP_PCM_INFO,
> +		.formats		= MMP_PCM_FORMATS,
> +		.period_bytes_min	= 1024,
> +		.period_bytes_max	= 2048,
> +		.periods_min		= 2,
> +		.periods_max		= 32,
> +		.buffer_bytes_max	= 4096,
> +		.fifo_size		= 32,
> +	},
> +	{
> +		.info			= MMP_PCM_INFO,
> +		.formats		= MMP_PCM_FORMATS,
> +		.period_bytes_min	= 1024,
> +		.period_bytes_max	= 2048,
> +		.periods_min		= 2,
> +		.periods_max		= 32,
> +		.buffer_bytes_max	= 4096,
> +		.fifo_size		= 32,
> +	},
> +};
> +
> +static void mmp_pcm_adma_irq(void *data)
> +{
> +	struct snd_pcm_substream *substream = data;
> +	struct mmp_runtime_data *prtd = substream->runtime->private_data;
> +	size_t dma_bytes = substream->runtime->dma_bytes;
> +
> +	prtd->pointer = (prtd->pointer + prtd->period_size) % dma_bytes;
> +	snd_pcm_period_elapsed(substream);
> +	return;
> +}
> +
> +static bool filter(struct dma_chan *chan, void *param)
> +{
> +	struct mmp_runtime_data *prtd = param;
> +	bool found = false;
> +	char *devname;
> +
> +	devname = kasprintf(GFP_KERNEL, "%s.%d", prtd->dma_res->name,
> +		prtd->ssp_id);
> +	if ((strcmp(dev_name(chan->device->dev), devname) == 0) &&
> +		(chan->chan_id == prtd->dma_res->start)) {
> +		chan->private = &prtd->tdma_data;
> +		found = true;
> +	}
> +
> +	kfree(devname);
> +	return found;
> +}
> +
> +static int mmp_pcm_hw_params(struct snd_pcm_substream *substream,
> +			      struct snd_pcm_hw_params *params)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct mmp_runtime_data *prtd = runtime->private_data;
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct pxa2xx_pcm_dma_params *dma_params;
> +	size_t totsize = params_buffer_bytes(params);
> +	size_t period = params_period_bytes(params);
> +	int ret;
> +	struct dma_slave_config slave_config;
> +	dma_cap_mask_t mask;
> +
> +	dma_params = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
> +	if (!dma_params)
> +		return 0;
> +
> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S8:
> +		prtd->tdma_data.bus_size = 8;
> +		break;
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		prtd->tdma_data.bus_size = 16;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		prtd->tdma_data.bus_size = 24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S32_LE:
> +		prtd->tdma_data.bus_size = 32;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	prtd->tdma_data.pack_mod = true;
> +
> +	dma_cap_zero(mask);
> +	dma_cap_set(DMA_CYCLIC, mask);
> +	prtd->dma_chan = dma_request_channel(mask, filter, prtd);
> +	if (!prtd->dma_chan)
> +		return -EINVAL;
> +
> +	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		slave_config.direction    = DMA_TO_DEVICE;
> +		slave_config.dst_addr     = dma_params->dev_addr;
> +		slave_config.dst_maxburst = 4;
> +	} else {
> +		slave_config.direction	  = DMA_FROM_DEVICE;
> +		slave_config.src_addr	  = dma_params->dev_addr;
> +		slave_config.src_maxburst = 4;
> +	}
> +
> +	ret = dmaengine_slave_config(prtd->dma_chan, &slave_config);
> +	if (ret)
> +		return ret;
> +
> +	snd_pcm_set_runtime_buffer(substream, &substream->dma_buffer);
> +
> +	prtd->period_size = period;
> +	runtime->dma_bytes = totsize;
> +
> +	prtd->desc = prtd->dma_chan->device->device_prep_dma_cyclic(
> +			prtd->dma_chan, runtime->dma_addr, totsize, period,
> +			substream->stream == SNDRV_PCM_STREAM_PLAYBACK ?
> +			DMA_TO_DEVICE : DMA_FROM_DEVICE, NULL);
> +	if (!prtd->desc) {
> +		dev_err(&prtd->dma_chan->dev->device, "cannot prepare slave dma\n");
> +		return -EINVAL;
> +	}
> +
> +	prtd->desc->callback = mmp_pcm_adma_irq;
> +	prtd->desc->callback_param = substream;
> +	dmaengine_submit(prtd->desc);
> +	return 0;
> +}
> +
> +static int mmp_pcm_hw_free(struct snd_pcm_substream *substream)
> +{
> +	struct mmp_runtime_data *prtd = substream->runtime->private_data;
> +
> +	if (prtd->dma_chan) {
> +		dma_release_channel(prtd->dma_chan);
> +		prtd->dma_chan = NULL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int mmp_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct mmp_runtime_data *prtd = substream->runtime->private_data;
> +	int ret = 0;
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_START:
> +		prtd->pointer = 0;
> +		dma_async_issue_pending(prtd->dma_chan);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_STOP:
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +		dmaengine_terminate_all(prtd->dma_chan);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		dma_async_issue_pending(prtd->dma_chan);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		dma_async_issue_pending(prtd->dma_chan);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static snd_pcm_uframes_t mmp_pcm_pointer(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct mmp_runtime_data *prtd = runtime->private_data;
> +	snd_pcm_uframes_t x;
> +
> +	x = bytes_to_frames(runtime, prtd->pointer);
> +	if (x == runtime->buffer_size)
> +		x = 0;
> +
> +	return x;
> +}
> +
> +static int mmp_pcm_open(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct platform_device *pdev = to_platform_device(rtd->platform->dev);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
> +	struct mmp_runtime_data *prtd;
> +	struct resource *r;
> +	int ret;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_DMA, substream->stream);
> +	if (!r)
> +		return -EBUSY;
> +
> +	snd_soc_set_runtime_hwparams(substream,
> +				&mmp_pcm_hardware[substream->stream]);
> +
> +	/*
> +	 * For mysterious reasons (and despite what the manual says)
> +	 * playback samples are lost if the DMA count is not a multiple
> +	 * of the DMA burst size.  Let's add a rule to enforce that.
> +	 */
> +	ret = snd_pcm_hw_constraint_step(runtime, 0,
> +		SNDRV_PCM_HW_PARAM_PERIOD_BYTES, 32);
> +	if (ret)
> +		goto out;
> +
> +	ret = snd_pcm_hw_constraint_step(runtime, 0,
> +		SNDRV_PCM_HW_PARAM_BUFFER_BYTES, 32);
> +	if (ret)
> +		goto out;
> +
> +	ret = snd_pcm_hw_constraint_integer(runtime,
> +					SNDRV_PCM_HW_PARAM_PERIODS);
> +	if (ret < 0)
> +		goto out;
> +
> +	prtd = devm_kzalloc(&pdev->dev,
> +			sizeof(struct mmp_runtime_data), GFP_KERNEL);
> +	if (prtd == NULL) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	prtd->substream = substream;
> +	runtime->private_data = prtd;
> +	prtd->dma_res = r;
> +	prtd->ssp_id = cpu_dai->id;
> +	return 0;
> +
> +out:
> +	return ret;
> +}
> +
> +static int mmp_pcm_close(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct platform_device *pdev = to_platform_device(rtd->platform->dev);
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	struct mmp_runtime_data *prtd = runtime->private_data;
> +
> +	devm_kfree(&pdev->dev, prtd);
> +	runtime->private_data = NULL;
> +	return 0;
> +}
> +
> +static int mmp_pcm_mmap(struct snd_pcm_substream *substream,
> +			 struct vm_area_struct *vma)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +	unsigned long off = vma->vm_pgoff;
> +
> +	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
> +	return remap_pfn_range(vma, vma->vm_start,
> +		__phys_to_pfn(runtime->dma_addr) + off,
> +		vma->vm_end - vma->vm_start, vma->vm_page_prot);
> +}
> +
> +struct snd_pcm_ops mmp_pcm_ops = {
> +	.open		= mmp_pcm_open,
> +	.close		= mmp_pcm_close,
> +	.ioctl		= snd_pcm_lib_ioctl,
> +	.hw_params	= mmp_pcm_hw_params,
> +	.hw_free	= mmp_pcm_hw_free,
> +	.trigger	= mmp_pcm_trigger,
> +	.pointer	= mmp_pcm_pointer,
> +	.mmap		= mmp_pcm_mmap,
> +};
> +
> +static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream)
> +{
> +	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
> +	struct snd_dma_buffer *buf = &substream->dma_buffer;
> +	size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
> +	struct gen_pool *gpool;
> +
> +	buf->dev.type = SNDRV_DMA_TYPE_DEV;
> +	buf->dev.dev = pcm->card->dev;
> +	buf->private_data = NULL;
> +
> +	gpool = sram_get_gpool("asram");
> +	if (!gpool)
> +		return -ENOMEM;
> +
> +	buf->area = (unsigned char *)gen_pool_alloc(gpool, size);
> +	if (!buf->area)
> +		return -ENOMEM;
> +	buf->addr = gen_pool_virt_to_phys(gpool, (unsigned long)buf->area);
> +	buf->bytes = size;
> +	return 0;
> +}
> +
> +static void mmp_pcm_free_dma_buffers(struct snd_pcm *pcm)
> +{
> +	struct snd_pcm_substream *substream;
> +	struct snd_dma_buffer *buf;
> +	int stream;
> +	struct gen_pool *gpool;
> +
> +	gpool = sram_get_gpool("asram");
> +	if (!gpool)
> +		return;
> +
> +	for (stream = 0; stream < 2; stream++) {
> +		size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
> +
> +		substream = pcm->streams[stream].substream;
> +		if (!substream)
> +			continue;
> +
> +		buf = &substream->dma_buffer;
> +		if (!buf->area)
> +			continue;
> +		gen_pool_free(gpool, (unsigned long)buf->area, size);
> +		buf->area = NULL;
> +	}
> +
> +	return;
> +}
> +
> +static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
> +
> +int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_card *card = rtd->card->snd_card;
> +	struct snd_pcm *pcm = rtd->pcm;
> +	int ret = 0;
> +
> +	if (!card->dev->dma_mask)
> +		card->dev->dma_mask = &mmp_pcm_dmamask;
> +
> +	if (!card->dev->coherent_dma_mask)
> +		card->dev->coherent_dma_mask = DMA_BIT_MASK(64);
> +
> +	if (pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream) {
> +		ret = mmp_pcm_preallocate_dma_buffer(pcm,
> +			SNDRV_PCM_STREAM_PLAYBACK);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	if (pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream) {
> +		ret = mmp_pcm_preallocate_dma_buffer(pcm,
> +			SNDRV_PCM_STREAM_CAPTURE);
> +		if (ret)
> +			goto out;
> +	}
> + out:
> +	return ret;
> +}
> +
> +struct snd_soc_platform_driver mmp_soc_platform = {
> +	.ops		= &mmp_pcm_ops,
> +	.pcm_new	= mmp_pcm_new,
> +	.pcm_free	= mmp_pcm_free_dma_buffers,
> +};
> +
> +static __devinit int mmp_pcm_probe(struct platform_device *pdev)
> +{
> +	struct mmp_audio_platdata *pdata = pdev->dev.platform_data;
> +
> +	if (pdata) {
> +		mmp_pcm_hardware[SNDRV_PCM_STREAM_PLAYBACK].buffer_bytes_max =
> +						pdata->buffer_max_playback;
> +		mmp_pcm_hardware[SNDRV_PCM_STREAM_PLAYBACK].period_bytes_max =
> +						pdata->period_max_playback;
> +		mmp_pcm_hardware[SNDRV_PCM_STREAM_CAPTURE].buffer_bytes_max =
> +						pdata->buffer_max_capture;
> +		mmp_pcm_hardware[SNDRV_PCM_STREAM_CAPTURE].period_bytes_max =
> +						pdata->period_max_capture;
> +	}
> +	return snd_soc_register_platform(&pdev->dev, &mmp_soc_platform);
> +}
> +
> +static int __devexit mmp_pcm_remove(struct platform_device *pdev)
> +{
> +	snd_soc_unregister_platform(&pdev->dev);
> +	return 0;
> +}
> +
> +static struct platform_driver mmp_pcm_driver = {
> +	.driver = {
> +		.name = "mmp-pcm-audio",
> +		.owner = THIS_MODULE,
> +	},
> +
> +	.probe = mmp_pcm_probe,
> +	.remove = __devexit_p(mmp_pcm_remove),
> +};
> +
> +module_platform_driver(mmp_pcm_driver);
> +
> +MODULE_AUTHOR("Leo Yan <leoy@marvell.com>");
> +MODULE_DESCRIPTION("MMP Soc Audio DMA module");
> +MODULE_LICENSE("GPL");


-- 
~Vinod

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Fri, May 25, 2012 at 01:23:36PM +0530, Vinod Koul wrote:
> On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
> > mmp-pcm handle audio dma based on dmaengine
> > Support mmp and pxa910
> Looks like this is *not* using soc-dmaengine library, why?

Note also...

> > +	prtd->dma_chan = dma_request_channel(mask, filter, prtd);
> > +	if (!prtd->dma_chan)
> > +		return -EINVAL;

This should be done at probe time, so we know the struct device, so
that...

> > +static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int stream)
> > +{
> > +	struct snd_pcm_substream *substream = pcm->streams[stream].substream;
> > +	struct snd_dma_buffer *buf = &substream->dma_buffer;
> > +	size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
> > +	struct gen_pool *gpool;
> > +
> > +	buf->dev.type = SNDRV_DMA_TYPE_DEV;
> > +	buf->dev.dev = pcm->card->dev;

... this uses the right device, and...

> > +static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
> > +
> > +int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd)
> > +{
> > +	struct snd_card *card = rtd->card->snd_card;
> > +	struct snd_pcm *pcm = rtd->pcm;
> > +	int ret = 0;
> > +
> > +	if (!card->dev->dma_mask)
> > +		card->dev->dma_mask = &mmp_pcm_dmamask;
> > +
> > +	if (!card->dev->coherent_dma_mask)
> > +		card->dev->coherent_dma_mask = DMA_BIT_MASK(64);

... we don't need crap like this.

Because then we'll be allocating buffers against the _right_ struct device
which is the DMA engine struct device.

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[zhangfei gao]

Thanks Russell and Vinod.

On Fri, May 25, 2012 at 4:05 PM, Russell King - ARM Linux <
linux@arm.linux.org.uk> wrote:

> On Fri, May 25, 2012 at 01:23:36PM +0530, Vinod Koul wrote:
> > On Fri, 2012-05-25 at 15:11 +0800, Zhangfei Gao wrote:
> > > mmp-pcm handle audio dma based on dmaengine
> > > Support mmp and pxa910
> > Looks like this is *not* using soc-dmaengine library, why?
>
> Note also...
>
Will look into soc-dmaengine.


>
> > > +   prtd->dma_chan = dma_request_channel(mask, filter, prtd);
> > > +   if (!prtd->dma_chan)
> > > +           return -EINVAL;
>
> This should be done at probe time, so we know the struct device, so
> that...
>

Do you mean at open time, like snd_dmaengine_pcm_open.
The channel resource is limited and better get dynamically.
As a  result the pcm_new and preallocate already called before.


> > > +static int mmp_pcm_preallocate_dma_buffer(struct snd_pcm *pcm, int
> stream)
> > > +{
> > > +   struct snd_pcm_substream *substream =
> pcm->streams[stream].substream;
> > > +   struct snd_dma_buffer *buf = &substream->dma_buffer;
> > > +   size_t size = mmp_pcm_hardware[stream].buffer_bytes_max;
> > > +   struct gen_pool *gpool;
> > > +
> > > +   buf->dev.type = SNDRV_DMA_TYPE_DEV;
> > > +   buf->dev.dev = pcm->card->dev;
>
> ... this uses the right device, and...
>
> > > +static u64 mmp_pcm_dmamask = DMA_BIT_MASK(64);
> > > +
> > > +int mmp_pcm_new(struct snd_soc_pcm_runtime *rtd)
> > > +{
> > > +   struct snd_card *card = rtd->card->snd_card;
> > > +   struct snd_pcm *pcm = rtd->pcm;
> > > +   int ret = 0;
> > > +
> > > +   if (!card->dev->dma_mask)
> > > +           card->dev->dma_mask = &mmp_pcm_dmamask;
> > > +
> > > +   if (!card->dev->coherent_dma_mask)
> > > +           card->dev->coherent_dma_mask = DMA_BIT_MASK(64);
>
> ... we don't need crap like this.
>
> Because then we'll be allocating buffers against the _right_ struct device
> which is the DMA engine struct device.
> --
> To unsubscribe from this list: send the line "unsubscribe alsa-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Fri, May 25, 2012 at 04:47:20PM +0800, zhangfei gao wrote:
> Do you mean at open time, like snd_dmaengine_pcm_open.
> The channel resource is limited and better get dynamically.
> As a  result the pcm_new and preallocate already called before.

This is where dealing with slave DMA channels in a virtualized setup
becomes a far better solution than trying to assign a particular
physical channel at request time.

What we may wish to think about is having a way for slave drivers to
assert to DMA engine the priority of a channel, which they can change
dynamically according to what they're doing.  Eg, an ALSA driver would
leave the channel low priority while it's not expecting to be used, but
as soon as we see the prepare call, set it to high priority.

The DMA engine driver could use that to decide to assign a physical
channel to the virtual channel, so that DMA can start as soon as
possible even with other activity on the DMA engine.

However, I've yet to see any setup where the number of physical DMA
channels available exceeds the number of actual _simultaneous_ users.
Even with the five channels on SA11x0 shared between 12? peripherals,
with my DMA engine driver I've only seen one or two physical channels
being used simultaneously.

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[zhangfei gao]

On Fri, May 25, 2012 at 5:42 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, May 25, 2012 at 04:47:20PM +0800, zhangfei gao wrote:
>> Do you mean at open time, like snd_dmaengine_pcm_open.
>> The channel resource is limited and better get dynamically.
>> As a  result the pcm_new and preallocate already called before.
>
> This is where dealing with slave DMA channels in a virtualized setup
> becomes a far better solution than trying to assign a particular
> physical channel at request time.
>
> What we may wish to think about is having a way for slave drivers to
> assert to DMA engine the priority of a channel, which they can change
> dynamically according to what they're doing.  Eg, an ALSA driver would
> leave the channel low priority while it's not expecting to be used, but
> as soon as we see the prepare call, set it to high priority.
>
> The DMA engine driver could use that to decide to assign a physical
> channel to the virtual channel, so that DMA can start as soon as
> possible even with other activity on the DMA engine.
>
> However, I've yet to see any setup where the number of physical DMA
> channels available exceeds the number of actual _simultaneous_ users.
> Even with the five channels on SA11x0 shared between 12? peripherals,
> with my DMA engine driver I've only seen one or two physical channels
> being used simultaneously.
>

When debugging with putting snd_dmaengine_pcm_open to probe or
pcm_new, there is issue.
snd_dmaengine_pcm_open ->
snd_pcm_hw_constraint_integer(substream->runtime,
                                            SNDRV_PCM_HW_PARAM_PERIODS);
The runtime is only exist at runtime.
So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has
to be put in open instead of probe.

Thanks

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Vinod Koul]

On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
> 
> When debugging with putting snd_dmaengine_pcm_open to probe or
> pcm_new, there is issue.
> snd_dmaengine_pcm_open ->
> snd_pcm_hw_constraint_integer(substream->runtime,
> 
> SNDRV_PCM_HW_PARAM_PERIODS);
> The runtime is only exist at runtime.
> So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has
> to be put in open instead of probe. 
I think you got it wrong, the snd_dmaengine_pcm_open needs to be called
from your CPU pcm_open. See other examples how this is used in other
drivers.

-- 
~Vinod

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
> On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
> > 
> > When debugging with putting snd_dmaengine_pcm_open to probe or
> > pcm_new, there is issue.
> > snd_dmaengine_pcm_open ->
> > snd_pcm_hw_constraint_integer(substream->runtime,
> > 
> > SNDRV_PCM_HW_PARAM_PERIODS);
> > The runtime is only exist at runtime.
> > So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has
> > to be put in open instead of probe. 
> I think you got it wrong, the snd_dmaengine_pcm_open needs to be called
> from your CPU pcm_open. See other examples how this is used in other
> drivers.

But, as I pointed out earlier, there's the issue of using the wrong
struct device to allocate memory for the DMA engine.  That's something
that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[zhangfei gao]

On Tue, May 29, 2012 at 3:33 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
>> On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
>> >
>> > When debugging with putting snd_dmaengine_pcm_open to probe or
>> > pcm_new, there is issue.
>> > snd_dmaengine_pcm_open ->
>> > snd_pcm_hw_constraint_integer(substream->runtime,
>> >
>> > SNDRV_PCM_HW_PARAM_PERIODS);
>> > The runtime is only exist at runtime.
>> > So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has
>> > to be put in open instead of probe.
>> I think you got it wrong, the snd_dmaengine_pcm_open needs to be called
>> from your CPU pcm_open. See other examples how this is used in other
>> drivers.
>
> But, as I pointed out earlier, there's the issue of using the wrong
> struct device to allocate memory for the DMA engine.  That's something
> that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.

Could you help clarify what's the struct device should be used?
Any example, not find soc-dmaengine.c.
When debug,
rtd->card->snd_card->dev is same as substream->pcm->card->dev, which
we using now.

Thanks a lot.

Re: [alsa-devel] [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Tue, May 29, 2012 at 03:57:18PM +0800, zhangfei gao wrote:
> On Tue, May 29, 2012 at 3:33 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Tue, May 29, 2012 at 10:48:54AM +0530, Vinod Koul wrote:
> >> On Tue, 2012-05-29 at 13:14 +0800, zhangfei gao wrote:
> >> >
> >> > When debugging with putting snd_dmaengine_pcm_open to probe or
> >> > pcm_new, there is issue.
> >> > snd_dmaengine_pcm_open ->
> >> > snd_pcm_hw_constraint_integer(substream->runtime,
> >> >
> >> > SNDRV_PCM_HW_PARAM_PERIODS);
> >> > The runtime is only exist at runtime.
> >> > So if using soc-dmaengine, I am afraid the snd_dmaengine_pcm_open has
> >> > to be put in open instead of probe.
> >> I think you got it wrong, the snd_dmaengine_pcm_open needs to be called
> >> from your CPU pcm_open. See other examples how this is used in other
> >> drivers.
> >
> > But, as I pointed out earlier, there's the issue of using the wrong
> > struct device to allocate memory for the DMA engine.  That's something
> > that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
> 
> Could you help clarify what's the struct device should be used?
> Any example, not find soc-dmaengine.c.
> When debug,
> rtd->card->snd_card->dev is same as substream->pcm->card->dev, which
> we using now.

When your intention is to DMA to/from some memory, that memory _should_
be mapped via the DMA API (or, in the case of dma_alloc_coherent,
allocated) _using_ the device associated with the agent performing the
DMA.

This is so that the DMA API can know whether there's an IOMMU that has
to be programmed for the DMA agent to be able to see the memory, or
whether there's restrictions on the range of RAM which is visible to
the DMA agent.

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Mark Brown]

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

On Tue, May 29, 2012 at 08:33:34AM +0100, Russell King - ARM Linux wrote:

> But, as I pointed out earlier, there's the issue of using the wrong
> struct device to allocate memory for the DMA engine.  That's something
> that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.

What is the issue with the current code - it *looks* like you want to
use the component device for the DMA as the struct device with dmaengine
but I don't understand the issue that's created if we don't (and why
things appear to be working for people as they are).

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

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

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:
> On Tue, May 29, 2012 at 08:33:34AM +0100, Russell King - ARM Linux wrote:
> 
> > But, as I pointed out earlier, there's the issue of using the wrong
> > struct device to allocate memory for the DMA engine.  That's something
> > that my soc-dmaengine.c got _right_, and soc-dmaengine-pcm.c gets _wrong_.
> 
> What is the issue with the current code - it *looks* like you want to
> use the component device for the DMA as the struct device with dmaengine
> but I don't understand the issue that's created if we don't (and why
> things appear to be working for people as they are).

Look.  It's very very very very simple.

What does the DMA API take?  A struct device.  What struct device?  Some
random struct device for something in the system, or what?  No, it takes
the struct device for the _device_ in the system which is _performing_
the DMA.

If your DMA is offloaded onto anther device, as is the case with DMA engine,
the _correct_ struct device to use with the DMA API is the DMA engine
device structure.  Because, if there's any restrictions on how that memory
is accessed - as I said in my other email - for instance, whether it be
that the DMA engine has an IOMMU in the way, or maybe has a restricted
view of memory - then your local struct device is the wrong one to be
using, because it will _not_ have that information to convey into the
DMA API.

Therefore, allocating or mapping DMA memory against one struct device,
and then passing it to an entirely different device in the system to
perform DMA on is _WRONG_.

It _may_ work as long as there are no restrictions or IOMMUs in the way,
because you'll happen to setup your local struct device the same as the
DMA engine struct device.  That's not always going to be the case.

If you persist in not wanting to care about this, then I'm afraid I'll
ignore soc-dmaengine-pcm.c entirely as to me its totally bolloxed code.
What I have as my own version (now with cyclic DMA support) is IMHO a
far superior and more correct implementation.

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Lars-Peter Clausen]

On 05/29/2012 11:21 AM, Russell King - ARM Linux wrote:
> [...]
> What I have as my own version (now with cyclic DMA support) is IMHO a
> far superior and more correct implementation.

Any chance you could post your code?

Thanks,
- Lars

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Mark Brown]

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

On Tue, May 29, 2012 at 10:21:29AM +0100, Russell King - ARM Linux wrote:
> On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:

> > What is the issue with the current code - it *looks* like you want to
> > use the component device for the DMA as the struct device with dmaengine
> > but I don't understand the issue that's created if we don't (and why
> > things appear to be working for people as they are).

> Look.  It's very very very very simple.

> What does the DMA API take?  A struct device.  What struct device?  Some
> random struct device for something in the system, or what?  No, it takes
> the struct device for the _device_ in the system which is _performing_
> the DMA.

I'm assuming that you mean the client rather than the DMA controller
itself (which we must already have found)?  That makes sense, 

> If you persist in not wanting to care about this, then I'm afraid I'll
> ignore soc-dmaengine-pcm.c entirely as to me its totally bolloxed code.
> What I have as my own version (now with cyclic DMA support) is IMHO a
> far superior and more correct implementation.

I'm sorry, I'm not sure what you mean when you say "if you persist"?
The mail I replied to was the first time I'd seen mention of this issue
at all.  I have to confess I haven't read every dmaengine thread in
detail, they're quite large and when they talk about generic issues they
seem to be focusing pretty much entirely on going over the issues with
the completion callbacks.

It would be enormously helpful if you could submit some code here (you
did once post a link to it but I don't recall it getting sent to the
list), probably as a replacement for the existing code if you don't want
to fix the existing code.  Even though API changes are going to be
required hopefully it should be relatively straightforward to convert
the existing users over to the new API, everything should at least be
following a common pattern so once one device is converted the rest
should be simple.  This seems much more likely to get us somewhere than
the current situation.

Currently we seem to have nobody actually working on this code in
mainline, as far as I can tell you and Vinod are the only people with
any active interest at a framework level (with Vinod's mostly being at
the dmaengine rather than ASoC level; I don't have any hardware with any
sort of dmaengine support) and because you have this out of tree
implementation you're working on you're mostly just offering feedback in
these driver review threads where apparently chunks of it are getting
missed.  Nobody else seems to be showing much inclination to get
involved so it seems unlikely to collide with other work.

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

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

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Russell King - ARM Linux]

On Tue, May 29, 2012 at 02:14:11PM +0100, Mark Brown wrote:
> On Tue, May 29, 2012 at 10:21:29AM +0100, Russell King - ARM Linux wrote:
> > On Tue, May 29, 2012 at 10:02:15AM +0100, Mark Brown wrote:
> 
> > > What is the issue with the current code - it *looks* like you want to
> > > use the component device for the DMA as the struct device with dmaengine
> > > but I don't understand the issue that's created if we don't (and why
> > > things appear to be working for people as they are).
> 
> > Look.  It's very very very very simple.
> 
> > What does the DMA API take?  A struct device.  What struct device?  Some
> > random struct device for something in the system, or what?  No, it takes
> > the struct device for the _device_ in the system which is _performing_
> > the DMA.
> 
> I'm assuming that you mean the client rather than the DMA controller
> itself (which we must already have found)?  That makes sense,

No, I mean the DMA controller *itself*.

DMA engines _can_ (and do - quite common on ARM platforms) have this setup:

                      CPU
                       |
               +-Bus switch matrix-+
     (RAM bus) |                   |
RAM------------+--DMA controller---+
                        |          |
                        |(rq+ack)  |(IO bus)
                        |          |
                        |          +-----------Peripheral 1
                        +--------- | ------------'
                                   +-----------Peripheral 2
                                  ...

So, the _peripherals_ themselves have no direct access to RAM at all.
These are totally and utterly incapable of any kind of DMA on their own.
They can only be _given_ data by some other agent in the system, that
being in the above case the CPU or the DMA controller.

When the DMA controller is setup, and asked to transfer data to a
peripheral device, it first starts by originating an access to RAM via
the RAM bus, and it loads the data into it's FIFO.  It then performs
accesses on the IO bus to _write_ the data into the peripheral just
as a CPU would under PIO.

So, to setup the _peripheral_ struct device with some nebulous "oh this
can access the whole of system memory and is DMA capable, let's allocate
DMA memory against it" is totally the wrong approach.  The peripheral
device can not know what the details are for some random DMA controller
elsewhere in the system.

Indeed, there are systems out there where the same peripheral can be
driven by more than one DMA controller.  What if each DMA controller
has different properties?

So, the struct device in the system representing the device which is
accessing memory is _the_ _DMA_ _controller_.

Why does this matter?  Consider this case:


                      CPU
                       |
     +-----------Bus switch matrix-+
     |                             |
RAM--+--IOMMU-----DMA controller---+
                        |          |
                        |(rq+ack)  |(IO bus)
                        |          |
                        |          +-----------Peripheral 1
                        +--------- | ------------'
                                   +-----------Peripheral 2
                                  ...

Now, for the DMA controller to access RAM, the IOMMU needs to be setup to
create mappings between the address space that the DMA controller can see,
and the RAM.  If there are no mappings setup, the DMA controller can't see
the RAM _at_ _all_ and DMA is not possible.  The IOMMU is managed via the
DMA API on many kernel architectures, x86 included.

If you use the peripheral struct device with the DMA API, how does the DMA
API know that it must setup some unknown DMA controller's IOMMU?

And why should the DMA API code even care about such complexities?

We don't do this on USB.  When a USB device wants to perform DMA, we map
its memory using the _host_ _controller_ struct device, not the USB
peripheral device's struct device.  It's exactly the same principle.
The struct device corresponding with whatever device is _actually_
accessing memory must be the struct device passed into the DMA API.

> Currently we seem to have nobody actually working on this code in
> mainline, as far as I can tell you and Vinod are the only people with
> any active interest at a framework level (with Vinod's mostly being at
> the dmaengine rather than ASoC level; I don't have any hardware with any
> sort of dmaengine support) and because you have this out of tree
> implementation you're working on you're mostly just offering feedback in
> these driver review threads where apparently chunks of it are getting
> missed.  Nobody else seems to be showing much inclination to get
> involved so it seems unlikely to collide with other work.

That's partly because you took in soc-dmaengine.c, after you ridiculed
my version - mainly because it didn't support cyclic DMA at the time.
And so I assumed that you'd made up your mind, and just plain weren't
interested.  You have been very resistive towards discussing issues
coming up in this area, and in other areas I've raised with ASoC such
as the struct device lifetime issues - even when I've created patches
for you.

Moreover, because I don't see any hope for the SA11x0 PCM support going
in (it certainly won't support capture mode due to the need to run the
playback side to receive capture samples) I'm really not pushing it; it's
more my testbed for testing out my DMA engine work than serious ASoC
work.

So please excuse me if I'm not in a rush to post it to the list.  In the
mean time, it's viewable via:

http://ftp.arm.linux.org.uk/git/gitweb.cgi?p=linux-arm.git;a=commitdiff;h=76bc4eb916528ceef2609c22f949429e2322a331

Re: [PATCH 2/4] ASoC: mmp: add audio dma support

[Mark Brown]

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

On Tue, May 29, 2012 at 02:46:32PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 29, 2012 at 02:14:11PM +0100, Mark Brown wrote:

> > I'm assuming that you mean the client rather than the DMA controller
> > itself (which we must already have found)?  That makes sense,

> No, I mean the DMA controller *itself*.

I'm quite famililar with the hardware, the issue here is about knowing
what the device is being used for.  In this case due to context being
lost in the quotes I hadn't realised exactly what the bug you were
identifying in the current code actually was, I had thought there was a
DMA channel already at this point but the lack of one is in fact the
problem.

You did identify this briefly on the original submission though as an
issue for sa11x0 (which you were higlighting is very oddball and
indicated you didn't really want in mainline) rather than a widespread
issue and the discussion (well, single followup from Lars-Peter) was
that this could be done incrementally which did seem resonable. After
that nobody did anything.

> That's partly because you took in soc-dmaengine.c, after you ridiculed
> my version - mainly because it didn't support cyclic DMA at the time.

I don't recall ever ridiculing your version, that's an extremely strong
term.  Can you please be more specific?  The nearest I can find to that
is Lars-Peter saying "I had a look at your 'generic' dmaengine PCM
driver patch" (which is rather dismissive).  I did say that the his idea
of moving the cyclic emulation down into dmaengine seemed sensible but I
did also agree that perhaps it was sensible to keep it further up the
stack if depending on other factors and I'm struggling to see this as
ridicule, and I did also express some displeasure at the SA11x0 hardware
design but that's definitely a separate thing.

In terms of the decision to apply the library code it's simply a case of
the fact that it was the only thing submitted, the only issue anyone had
with it was the allocation issue which seemed fixable incrementally, and
it was posted with all the relevant mainline platforms converted over.
This seems like a clear win, providing a noticable improvement on the
previous situation.

Having the library doesn't stop anyone creating a generic DMA driver on
top of it or otherwise fixing or improving on the merged code and it
does provide useful code sharing to the platforms.

> And so I assumed that you'd made up your mind, and just plain weren't
> interested.

I think you're reading something into things that just isn't there.  You
seem to see this as an either/or thing but really as with everything
else in the kernel the current code is just a useful point at which to
start doing incremental development in tree.

As I said at the time I think the final result should be a merge of the
two sets of code, and the addition of cyclic DMA support to your code is
one example of this happening though sadly out of mainline.  Looking at
the current state of things the changes I'd hope to see happen if
anyone's willing to work on it are something like (in no real order):

 - Move the channel acquisition earlier and use that device to allocate
   buffers.
 - Merge the non-cyclic support into mainline.
 - Add the generic driver into mainline in some form or other (or
   otherwise factor even more code out into the generic code).
 - Provide the ability for DAIs to register an ASoC DMA driver for
   themselves from data (like the dmaengine based DT systems want to
   do).
 - Give platforms more control over the parameters for the generic
   driver than they currently do (there's some stuff still #defined in
   the file).

Possibly split up differently somehow but broadly speaking that's where
I'd like to see us going.  Hopefully some or all of this will happen
before I have a dmaengine based platform running mainline, but if not I
guess I'll do some stuff myself.

In short there's clearly room for improvement in what's in mainline,
let's fix it with code.

>              You have been very resistive towards discussing issues
> coming up in this area, and in other areas I've raised with ASoC such
> as the struct device lifetime issues - even when I've created patches
> for you.

The one case I can recall not applying a patch you've sent was those
device lifetime things where my point was that unless there's an issue
being seen in actual systems that really isn't the sort of thing to be
fixing in -rc (which was what you were pushing strongly for), especially
when the patch only addresses a subset of the issue.  I have to say I am
rather conservative about what I'll apply during -rc, especially to core
code and around areas of code like that which are poorly tested
minefields.

Besides, the fix should be done at AC'97 bus level anyway since the
non-ASoC AC'97 code follows exactly the same pattern - if it's worth
fixing it's worth fixing for non-ASoC AC'97 systems too.

Otherwise for things other than patches you have to be aware that, in
common with most other maintainers, I've got a limited amount of time to
work on things and a whole bunch of different priorities both within the
kernel and in the rest of my life so if you're asking for something
that's hard or time consuming it might not happen immediately.  There
are a bunch of other people who work full time on ASoC things with a
similar mandate to me you can ask as well.

> Moreover, because I don't see any hope for the SA11x0 PCM support going
> in (it certainly won't support capture mode due to the need to run the
> playback side to receive capture samples) I'm really not pushing it; it's
> more my testbed for testing out my DMA engine work than serious ASoC
> work.

I really don't recall anything fundamental that would block it at least
for a playback only driver.  There were some issues with L3 but you
explained what they were and otherwise it just seemed to be updating to
current APIs and what looked like fairly straightforward code motion
stuff.

Doing capture might be more tricky but if the bodge is well enough
hidden in the driver or sufficiently non-invasive to the core it
shouldn't be that big a deal.

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

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

Re: [PATCH 3/4] ASOC: mmp: add sspa support

[Mark Brown]

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

On Fri, May 25, 2012 at 03:11:02PM +0800, Zhangfei Gao wrote:
> The SSPA is a configurable multi-channel audio serial (TDM) interface.
> It's configurable at runtime to support up to 128 channels and the
> number of bits per sample: 8, 12, 16, 20, 24 and 32 bits. It also
> support stereo format: I2S, left-justified or right-justified.

Mostly looks good.  A few fairly minor things...

> +static int mmp_sspa_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->cpu_dai;
> +	struct sspa_priv *sspa_priv = snd_soc_dai_get_drvdata(dai);
> +	struct ssp_device *sspa = sspa_priv->sspa;
> +	int ret = 0;
> +
> +	if (!cpu_dai->active) {
> +		clk_enable(sysclk);
> +		clk_enable(sspa->clk);
> +	}

The clock API is refcounted so you shouldn't need to worry about
multiple enables, you should just be able to unconditionally enable and
disable.  If this is needed we probably have a problem we should fix.

> +	switch (pll_id) {
> +	case MMP_SYSCLK:
> +		clk_set_rate(sysclk, freq_out);
> +		break;
> +	case MMP_SSPA_CLK:
> +		clk_set_rate(sspa->clk, freq_out);
> +		break;

You're ignoring the return values here.

> +	priv->sspa->clk = clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->sspa->clk)) {
> +		ret = PTR_ERR(priv->sspa->clk);
> +		goto err_free_clk;
> +	}

devm_clk_get().

> +	res = request_mem_region(res->start, resource_size(res),
> +			pdev->name);
> +	if (res == NULL) {
> +		dev_err(&pdev->dev, "failed to request memory resource\n");
> +		ret = -EBUSY;
> +		goto err_get_res;
> +	}
> +
> +	priv->sspa->mmio_base = ioremap(res->start, resource_size(res));
> +	if (priv->sspa->mmio_base == NULL) {
> +		dev_err(&pdev->dev, "failed to ioremap() registers\n");
> +		ret = -ENODEV;
> +		goto err_free_mem;
> +	}

devm_request_and_ioremap().

> +err_free_clk:
> +	devm_kfree(&pdev->dev, priv->dma_params);
> +err_priv_dma_params:
> +	devm_kfree(&pdev->dev, priv->sspa);
> +err_priv_sspa:
> +	devm_kfree(&pdev->dev, priv);

The whole point of the devm_ stuff is that you don't need to do stuff
like this.

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

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

Re: [PATCH 3/4] ASOC: mmp: add sspa support

[zhangfei gao]

On Mon, May 28, 2012 at 10:59 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, May 25, 2012 at 03:11:02PM +0800, Zhangfei Gao wrote:
>> The SSPA is a configurable multi-channel audio serial (TDM) interface.
>> It's configurable at runtime to support up to 128 channels and the
>> number of bits per sample: 8, 12, 16, 20, 24 and 32 bits. It also
>> support stereo format: I2S, left-justified or right-justified.
>
> Mostly looks good.  A few fairly minor things...
Thanks Mark for kind review.


>> +
>> +     if (!cpu_dai->active) {
>> +             clk_enable(sysclk);
>> +             clk_enable(sspa->clk);
>> +     }
>
> The clock API is refcounted so you shouldn't need to worry about
> multiple enables, you should just be able to unconditionally enable and
> disable.  If this is needed we probably have a problem we should fix.
Will update.

>
>> +     switch (pll_id) {
>> +     case MMP_SYSCLK:
>> +             clk_set_rate(sysclk, freq_out);
>> +             break;
>> +     case MMP_SSPA_CLK:
>> +             clk_set_rate(sspa->clk, freq_out);
>> +             break;
>
> You're ignoring the return values here.
will update.

>
>> +     priv->sspa->clk = clk_get(&pdev->dev, NULL);
>> +     if (IS_ERR(priv->sspa->clk)) {
>> +             ret = PTR_ERR(priv->sspa->clk);
>> +             goto err_free_clk;
>> +     }
>
> devm_clk_get().
Unfortunately, not find devm_clk_get.

>
>> +     res = request_mem_region(res->start, resource_size(res),
>> +                     pdev->name);
>> +     if (res == NULL) {
>> +             dev_err(&pdev->dev, "failed to request memory resource\n");
>> +             ret = -EBUSY;
>> +             goto err_get_res;
>> +     }
>> +
>> +     priv->sspa->mmio_base = ioremap(res->start, resource_size(res));
>> +     if (priv->sspa->mmio_base == NULL) {
>> +             dev_err(&pdev->dev, "failed to ioremap() registers\n");
>> +             ret = -ENODEV;
>> +             goto err_free_mem;
>> +     }
>
> devm_request_and_ioremap().
will update.
>
>> +err_free_clk:
>> +     devm_kfree(&pdev->dev, priv->dma_params);
>> +err_priv_dma_params:
>> +     devm_kfree(&pdev->dev, priv->sspa);
>> +err_priv_sspa:
>> +     devm_kfree(&pdev->dev, priv);
>
> The whole point of the devm_ stuff is that you don't need to do stuff
> like this.

Thanks confirmation, not sure before, also find devm_kfree is used.
Will remove the devm_kfree etc in err handle as well as remove function.

>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>

Re: [PATCH 4/4] ASoC: add mmp brownstone support

[Mark Brown]

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

On Fri, May 25, 2012 at 03:11:03PM +0800, Zhangfei Gao wrote:

> +config SND_MMP_SOC_BROWNSTONE
> +	tristate "SoC Audio support for Marvell Brownstone"
> +	depends on SND_MMP_SOC && MACH_BROWNSTONE
> +	select SND_MMP_SOC_SSPA
> +	select SND_SOC_WM8994

Should depend on MFD_WM8994.

> +static void brownstone_ext_control(struct snd_soc_dapm_context *dapm)
> +{

This stuff is really unexpected in a modern machine driver, it was used
on things like spitz due to the odd wiring but for modern stuff I'd not
expect to see it.

> +	if (brownstone_spk_func == BROWNSTONE_SPK_ON) {
> +		snd_soc_dapm_enable_pin(dapm, "Ext Left Spk");
> +		snd_soc_dapm_enable_pin(dapm, "Ext Right Spk");
> +	} else {
> +		snd_soc_dapm_disable_pin(dapm, "Ext Left Spk");
> +		snd_soc_dapm_disable_pin(dapm, "Ext Right Spk");
> +	}

Just define a single widget for the speakers and use a
SND_SOC_DAPM_PIN_SWITCH().

> +	/* set up jack connection */
> +	switch (brownstone_jack_func) {
> +	case BROWNSTONE_HP:
> +		snd_soc_dapm_disable_pin(dapm, "Headset Mic");
> +		snd_soc_dapm_enable_pin(dapm, "Main Mic");
> +		snd_soc_dapm_enable_pin(dapm, "Headset Stereophone");
> +		break;

This should all be autodetectable, users having to manually select it is
*very* unusual.  Is there really no accessory detection hardware on the
board?

> +	snd_soc_dapm_enable_pin(dapm, "Ext Left Spk");
> +	snd_soc_dapm_enable_pin(dapm, "Ext Right Spk");
> +	snd_soc_dapm_enable_pin(dapm, "Headset Stereophone");
> +	snd_soc_dapm_enable_pin(dapm, "Headset Mic");
> +	snd_soc_dapm_enable_pin(dapm, "Main Mic");

Everything is enable dby default.

> +	/* turn on micbias 1/2 always */
> +	snd_soc_update_bits(codec, WM8994_POWER_MANAGEMENT_1,
> +			    WM8994_MICB1_ENA_MASK |
> +			    WM8994_MICB2_ENA_MASK,
> +			    WM8994_MICB1_ENA |
> +			    WM8994_MICB2_ENA);

If you need to do this force enable them with DAPM, this won't work
anyway as the widgets will be powered off as soon as DAPM notices
they're on.

> +	snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_I2S |
> +		SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);
> +
> +	snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S |
> +		SND_SOC_DAIFMT_NB_NF | SND_SOC_DAIFMT_CBS_CFS);

Set this in the dai_link.

> +static struct platform_driver mmp_driver = {
> +	.driver		= {
> +		.name	= "mmp-audio",

Should probably be something like "brownstone-audio".

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

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